qprofiler1 / qvisualise - handle new fastq header #336
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ore robust to deal with these 2 issues here, qprofiler was not aware of this new fastq header format and so each header became a unique instrument (all 28 million of them). Qvisualise would then run out of memory trying to deal with them all. HAvave added code to deprperly deal with the new format, in qprofiler, and perhaps more importantly, addeded some limits to the size of collections that can be handled by qvisualise.
delocalizer
approved these changes
Jul 17, 2023
Contributor
delocalizer
left a comment
delocalizer
left a comment
There was a problem hiding this comment.
Looks good. A couple of minor changes suggested.
| String [] firstElementParams = params[0].split(" "); | ||
| if (firstElementParams.length != 2) { | ||
| throw new UnsupportedOperationException("Incorrect header format encountered in parseFiveElementHeader. Expected '@ERR091788.3104 HSQ955_155:2:1101:13051:2071/2' but recieved: " + Arrays.deepToString(params)); | ||
| throw new UnsupportedOperationException("Incorrect header format encountered in parseFiveElementHeader. Expected '@ERR091788.3104 HSQ955_155:2:1101:13051:2071/2' but received: " + Arrays.deepToString(params)); |
Contributor
There was a problem hiding this comment.
Here and ll 299, 306 the Exception messages are a bit confusing because they refer not to a generic format but to a specific example as 'Expected'; is there a better wording for these?
| logger.info("supplied Xmx memory: " + xmxSize); | ||
| double ratio = (double)xmxSize / fileSize; | ||
| logger.info("memory / file size ratio: " + ratio); | ||
| if (ratio < 8) { |
Contributor
There was a problem hiding this comment.
add some comment about this heuristic?
| map.put(isInteger ? (T) Integer.valueOf(tallyItemElement.getAttribute("value")) | ||
| : (T) tallyItemElement.getAttribute("value") , new AtomicLong(count)); | ||
|
|
||
| if (tallyItemsNL.getLength() < 100000) { |
Contributor
There was a problem hiding this comment.
magic number -> constant
| } | ||
| } | ||
| } else { | ||
| log.warn(cycleElement.getTagName() + " has too many elements: " + tallyItemsNL.getLength() + " - will leave out of report"); |
Contributor
There was a problem hiding this comment.
and refer to the exceeded constant value here in the log message
…as per reviewer suggestions
delocalizer
approved these changes
Jul 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix(qvisualise/qprofiler): deal with new fastq header and make qvis more robust to deal with these
2 issues here,
qprofilerwas not aware of this newfastqheader format and so each header became a unique instrument (all 28 million of them).Qvisualisewould then run out of memory trying to dealwith them all. Have added code to properly deal with the new format, in
qprofiler, andperhaps more importantly, added some limits to the size of collections that can be handled by
qvisualise.New fastq header:
@SRR14585604.8 A00805:41:HMJJWDRXX:1:1101:16929:1000 length=101which looks like the NCBI header as described here
What
qprofilerwill now do for this is to count the number of spaces in the header. If there are 2, it will just send the middle part to be analysed. For any other number of spaces, the header will be treated as it was previously.qvisualisewill now run some checks on the input xml file size along with the supplied Xmx parameter, and log if it thinks a memory issue may be encountered.It will also ignore any xml element that has more than 100000 items in it.
Type of change
How Has This Been Tested?
Additional unit test have been added.
Code has been run against offending fastq file and has produced satisfactory results
Are WDL Updates Required?
Nope
Checklist: